Fix memory corruption and support WinPE#841
Fix memory corruption and support WinPE#841eransha-salvador wants to merge 7 commits intoPowerShell:latestw_allfrom
Conversation
- Prefer VS 2017/2019/2022 over newer installs (e.g. VS 2026) when vswhere reports both, so a host with a newer VS installed alongside VS 2022 still builds with v143. Without this the downstream Select-String version checks miss and the VS 2015 fallback dereferences a null VS140COMNTOOLS. - Pin vcpkg's CMake with VCPKG_VISUAL_STUDIO_PATH and VCPKG_PLATFORM_TOOLSET=v143 so it uses the same toolset as the .vcxproj files, avoiding MSB8040 (Spectre-libs missing for v145) when vcpkg would otherwise pick up a newer VS. - Fall back to the repo root for OpenSSH-build.ps1 -destination when \$env:WORKSPACE is unset. CI still has its WORKSPACE value, and no longer invokes this script directly anyway. - Document Windows build prerequisites in README.txt: VS 2022 Desktop C++ workload, v143 Spectre-mitigated libs, Git, the one-time vcpkg bootstrap/integrate step, and the need to run from an elevated PowerShell.
|
If it's only a transitive reference, does removing the dependency on the DLL from sshd-auth, instead of delay loading, work? |
|
@tgauth it won't work.
|
The .vcxproj files pin PlatformToolset v143, which ships with VS 2022. VS 2017 (v141) and VS 2019 (v142) would need v143 build tools sideloaded to build this, which is a non-default setup — so they shouldn't be treated as equals to VS 2022 in the preference order.
eransha-salvador
force-pushed
the
839-fix-memory-corruption-and-support-winpe
branch
from
2887388 to
f98e834
Compare
The debug3 on LoadUserProfileW failure had format "%s %S %d" (three specifiers) but only two arguments. %S consumed GetLastError()'s DWORD as a wide-string pointer and wcsnlen dereferenced it, crashing sshd-session post-auth. Only visible where LoadUserProfileW actually fails (e.g. WinPE, which has no user-profile service), so regular Windows was unaffected.
sshd-auth runs as a privsep helper under a different user with no desktop/window-station access. user32's DllMain binds to the process window station and fails with STATUS_DLL_INIT_FAILED in restricted environments like WinPE, crashing the helper before auth can run. Add delayimp.lib and /DELAYLOAD:user32.dll so user32 is only loaded when one of its APIs is actually called. sshd-auth's only transitive user32 references come from console.c's ConRestoreViewRect_NoPtyHack (ShowWindow / GetWindowPlacement), which sshd-auth never executes, so in practice user32 is never loaded at all. The ItemDefinitionGroup carrying /DELAYLOAD is placed after Microsoft.Cpp.props, where the Link item type becomes defined.
eransha-salvador
force-pushed
the
839-fix-memory-corruption-and-support-winpe
branch
from
f98e834 to
a650ee0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@eransha-salvador, are you creating a local user account to attempt authentication with? |
@tgauth I did - on winpe I'm creating a local user account to login with. |
KERB_S4U_LOGON requires a KDC, so it cannot succeed on a workgroup
machine. Prefixed names ("DOMAIN\user") were being routed to the
Kerberos package regardless of host join state — failing on WinPE
(whose SAM hive retains a "minwinpc" domain that diverges from the
runtime computer name) and on any non-joined host where a client
happens to send a qualified username.
Gate Kerberos selection on NetGetJoinInformation. When the host is
not domain-joined, route through MSV1_0 and skip past any leading
backslash so the bare SAM name reaches LsaLogonUser. The auth
package and message struct are now selected by the same predicate,
so they cannot disagree.
Also add a debug3 in get_passwd surfacing the domain/computer name
pair, to make future SAM-vs-GetComputerName divergences easier to
diagnose.
@tgauth - the last commit handles the issue with public key authentication. |
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific crash/memory corruption in profile-loading error logging and improves WinPE/restricted-session compatibility by preventing sshd-auth from eagerly loading user32.dll. Also updates the Windows build helper/scripts and build documentation to better align with the v143/VS2022 toolchain and vcpkg manifest behavior.
Changes:
- Fixes a format-string argument mismatch in
load_user_profile()and adjusts S4U auth-package selection to use Kerberos only when the machine is domain-joined. - Updates
sshd-auth.vcxprojto delay-loaduser32.dll(and linkdelayimp.lib) to avoidSTATUS_DLL_INIT_FAILEDin restricted window stations (e.g., WinPE). - Updates build docs and build helper logic to prefer VS 2022/v143 and set vcpkg toolset-related environment variables.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| contrib/win32/win32compat/win32_usertoken_utils.c | Adds domain-join detection to gate Kerberos usage; fixes LoadUserProfileW debug logging arg mismatch. |
| contrib/win32/openssh/sshd-auth.vcxproj | Adds delayimp.lib and /DELAYLOAD:user32.dll to avoid loading user32 in restricted environments. |
| contrib/win32/openssh/README.txt | Adds build prerequisites and guidance for VS2022/vcpkg/administrator requirements. |
| contrib/win32/openssh/OpenSSHBuildHelper.psm1 | Prefers VS 2022 and pins vcpkg environment to v143/selected VS installation. |
| contrib/win32/openssh/OpenSSH-build.ps1 | Makes $destination default robust when WORKSPACE isn’t set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Make is_domain_joined_machine static (TU-local) and reformat to match the file's brace/indent conventions. - Remove the README claim that the build script adds Git's cmd directory to the machine PATH; no such logic exists in the build scripts.
Per PR review: replacing $(AdditionalDependentLibs) with a hand-written list made sshd-auth's link inputs drift from the rest of the solution (and dropped libs supplied by paths.targets). Append delayimp.lib to the inherited value instead. user32.lib is already in the inherited list; the /DELAYLOAD:user32.dll directive is unchanged, so delay-load behavior is preserved.
Summary
load_user_profile(%s %S %dwith two args) that corrupts memory and crashes sshd-session post-auth whenLoadUserProfileWfails.user32.dllin sshd-auth so it survives restricted window stations (e.g. WinPE) whereuser32'sDllMainfails withSTATUS_DLL_INIT_FAILED. The only transitive user32 references (ShowWindow / GetWindowPlacement via ConRestoreViewRect_NoPtyHack) are unreachable in sshd-auth, so the DLL is never loaded in practice.Stacked on #840 — please merge that first. Until #840 lands, this PR's diff view also shows its commit; after #840 merges, this PR will rebase down to just the two commits above.
Fixes PowerShell/Win32-OpenSSH#2435
Test plan